Walkthrough

The concat function in datafusion/functions/src/string/concat.rs has been modified to support array-aware concatenation. The signature initialization changed from Signature::variadic to Signature::variadic_any. Two new private helper methods were added: has_array_args to detect array arguments and to_array_concat to construct ArrayConcat expressions. The return_type method now handles array inputs by returning array types when List arguments are present. The invoke_with_args method was extended to detect and route array cases through the new array concatenation path while maintaining backward compatibility with scalar concatenation logic. Related imports were updated to support these changes.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Code Review for PR #113: fix: apache#10020 make concat function support array concatenation

Thank you for working on this feature! I've reviewed the changes and identified several critical issues that need to be addressed before this can be merged.

🔴 Critical Issues

1. Compilation Error - Missing closing brace (Line 72)

Self {
    signature: Signature::variadic_any(Volatility::Immutable)
    ),  // ❌ Missing opening brace, extra closing paren
}

Should be:

Self {
    signature: Signature::variadic_any(Volatility::Immutable),
}

2. Missing closing brace for impl block (Line 107)

The to_array_concat helper function is defined inside the impl ConcatFunc block, but the impl block is never closed before starting impl ScalarUDFImpl for ConcatFunc on line 108. You need to add a closing brace } after line 106.

3. Non-existent Methods Used (Lines 151-153)

let exprs: Vec<Expr> = args.args.into_iter().map(|a| a.into_expr()).collect();
let expr = Self::to_array_concat(exprs)?;
return expr.eval(args.context);

Problems:

• ColumnarValue does not have an into_expr() method in the DataFusion codebase
• Expr does not have an eval() method that takes a context parameter
• This approach fundamentally won't work - you cannot convert runtime ColumnarValue back to Expr and evaluate it

4. Incorrect Use of BuiltinScalarFunction (Lines 87, 92-94)

let array_concat = BuiltinScalarFunction::ArrayConcat;
// ...
Ok(Expr::ScalarFunction(ScalarFunction::new(
    BuiltinScalarFunction::MakeArray,
    vec\![arg],
)))

Problems:

• BuiltinScalarFunction enum no longer exists in modern DataFusion (removed in v27+)
• Array functions like array_concat are now implemented as UDFs (User Defined Functions), not built-in scalar functions
• You should be calling the actual array_concat UDF from datafusion-functions-nested crate

🟡 Design & Architecture Concerns

5. Fundamental Architectural Issue

The approach of converting ColumnarValue → Expr → evaluate is not possible in DataFusion's architecture:

• During execution (invoke_with_args), you have runtime values (ColumnarValue)
• You cannot convert these back to expressions (Expr) which are planning-time constructs
• You need to call the array_concat function's implementation directly, not build and evaluate an expression

Suggested approach:

// Get the array_concat UDF
let array_concat_udf = datafusion_functions_nested::concat::array_concat();

// Call it directly with the args
return array_concat_udf.invoke_with_args(args);

6. Signature Change Too Permissive (Line 71)

Changing from:

Signature::variadic(vec\![Utf8View, Utf8, LargeUtf8], Volatility::Immutable)

to:

Signature::variadic_any(Volatility::Immutable)

This allows any types, which could cause issues. Consider using a signature that explicitly allows both string types AND list types.

7. Missing Type Coercion Logic

When mixing scalar values with arrays (e.g., concat('a', [1, 2])), how should type coercion work? The current implementation doesn't handle this properly.

🟠 Other Issues

8. Unused Imports (Lines 18-20, 30-31, 33)

use arrow::array::{Array, as_largestring_array, ArrayRef};
use arrow::datatypes::{DataType, Field, TimeUnit};  // TimeUnit unused
use arrow_array::types::*;  // Wildcard import, nothing used
use datafusion_common::{Result, ScalarValue, internal_err, plan_err, exec_err};  // exec_err unused
use datafusion_expr::expr::{ScalarFunction, ScalarFunctionExpr};  // ScalarFunctionExpr unused
use datafusion_expr::{ColumnarValue, Documentation, Expr, Volatility, lit, BuiltinScalarFunction};  // BuiltinScalarFunction doesn't exist

9. Return Type Logic May Be Incorrect (Lines 124-130)

When any argument is a List, you return the first List type found. But what if there are multiple List types with different element types? This could cause type mismatches.

10. No Tests Added

This is a significant behavioral change, but I don't see any new tests for:

• Array concatenation: concat([1,2], [3,4])
• Mixed scalar and array: concat('hello', ['world'])
• Edge cases: empty arrays, null arrays, etc.

📋 Recommendations

1. Fix compilation errors first (issues 18481: feat: Add selectivity metric to NestedLoopJoinExec for EXPLAIN ANALYZE #1 and 18457: Support Arrow IPC Stream Files #2)
2. Rework the implementation approach:
   • Instead of trying to convert to expressions, directly invoke the array_concat UDF
   • Look at how other functions delegate to different implementations based on types
   • Consider whether concat should support arrays at all, or if users should use array_concat directly
3. Add comprehensive tests covering all new functionality
4. Consider backward compatibility: Does this change break existing behavior?
5. Update documentation to explain array concatenation behavior

📚 References

• Array concat implementation: datafusion/functions-nested/src/concat.rs
• Example of UDF invocation patterns in DataFusion
• Consider looking at how other dual-purpose functions handle different types

Please let me know if you need help with any of these issues. The concept of supporting array concatenation is good, but the implementation needs significant rework to align with DataFusion's architecture.

1. Compilation Error - Missing closing brace (Line 72)

Self {
    signature: Signature::variadic_any(Volatility::Immutable)
    ),  // ❌ Missing opening brace, extra closing paren
}

Should be:

Self {
    signature: Signature::variadic_any(Volatility::Immutable),
}

value:useful; category:bug; feedback:The Claude AI reviewer is correct! There is one more closing brace ()) that has left from the old implementation. Prevents compilation error.

2. Missing closing brace for impl block (Line 107)

The to_array_concat helper function is defined inside the impl ConcatFunc block, but the impl block is never closed before starting impl ScalarUDFImpl for ConcatFunc on line 108. You need to add a closing brace } after line 106.

value:useful; category:bug; feedback:The Claude AI reviewer is correct! There is a missing closing curly bracket (}) to close the previous impl block. Prevents compilation error.

3. Non-existent Methods Used (Lines 151-153)

let exprs: Vec<Expr> = args.args.into_iter().map(|a| a.into_expr()).collect();
let expr = Self::to_array_concat(exprs)?;
return expr.eval(args.context);

Problems:

• ColumnarValue does not have an into_expr() method in the DataFusion codebase
• Expr does not have an eval() method that takes a context parameter
• This approach fundamentally won't work - you cannot convert runtime ColumnarValue back to Expr and evaluate it

value:useful; category:bug; feedback:The Claude AI reviewer is correct! It seems this code is AI-generated and not reviewed by the PR author. The proposed changes are full with compilation errors.

There is no point in reviewing this PR! It is AI-generated and not reviewed/polished by the PR author.